-
Notifications
You must be signed in to change notification settings - Fork 3
Use installation/repositories API for searching repos
#53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe adapter replaces the GitHub search endpoint Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/VCS/Adapter/Git/GitHub.php`:
- Line 120: The array_filter call assigning $repositories uses case-sensitive
str_contains and the redundant null-coalescing ?? []; update the filter to use
case-insensitive matching via stripos (i.e., replace str_contains with a check
that stripos($repo['name'], $search) !== false) and remove the ?? [] so it
directly iterates $response['body']['repositories'] (which is already validated
earlier); keep the $repositories variable and array_filter call but swap the
matcher and drop the redundant fallback.
- Around line 109-125: The current implementation calls
$this->call(self::METHOD_GET, $url, ...) to fetch a single page and then filters
$response['body']['repositories'] locally, which breaks pagination and makes
'total' inaccurate; modify the logic in GitHub.php so that when $search is
non-empty you page through all results from $this->call(self::METHOD_GET, $url,
['Authorization' => "Bearer $this->accessToken"], ...) (looping with 'page'
param until no more items), accumulate all repositories, apply the local filter
(str_contains on $repo['name']), then return the filtered slice and accurate
total; keep single-page behavior when $search is empty to preserve performance.
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
107-107: Remove the unused$ownerparameter or update the endpoint to use owner-based filtering.The
$ownerparameter is declared in the method signature and documented in the abstract definition, but it's never used in the implementation since the method uses the/installation/repositoriesendpoint, which lists all repositories accessible to the GitHub App installation regardless of owner. Either remove the parameter to match the current endpoint behavior, or change the endpoint to use owner-specific filtering (e.g.,/users/{owner}/reposor/orgs/{owner}/repos).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 119-121: The code currently breaks out when
(!isset($response['body']['repositories'])) which silences API errors and can
return incomplete data; replace the break with throwing an exception (e.g.,
RuntimeException or a VCS-specific exception) from the GitHub adapter
(file/class GitHub.php) at the check for $response['body']['repositories'],
including contextual details such as the full $response payload (or status code
and error message) and the current request/page info so callers can surface and
handle the API error instead of silently continuing.
♻️ Duplicate comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
123-131: Stop condition uses filtered count; can truncate search results.If a page has <100 matches but more repositories exist, the loop ends early and misses later matches. Use the unfiltered page size to decide when to stop.
✅ Suggested fix
- // Filter repositories to only include those that match the search query. - $filteredRepositories = array_filter($response['body']['repositories'], fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false); + $pageRepositories = $response['body']['repositories']; + // Filter repositories to only include those that match the search query. + $filteredRepositories = array_filter($pageRepositories, fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false); // Merge with result so far. $repositories = array_merge($repositories, $filteredRepositories); - // If less than 100 repositories are returned, we have fetched all repositories. - if (\count($filteredRepositories) < 100) { + // If less than 100 repositories are returned, we have fetched all repositories. + if (\count($pageRepositories) < 100) { break; }
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
109-143: Avoid full scan when$searchis empty (perf regression).When no filter is needed, a single paged request is enough and avoids N+1 API calls.
♻️ Suggested refactor
public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array { + if (empty($search)) { + $response = $this->call(self::METHOD_GET, '/installation/repositories', ['Authorization' => "Bearer $this->accessToken"], [ + 'page' => $page, + 'per_page' => $per_page, + ]); + if (!isset($response['body']['repositories'])) { + $status = $response['headers']['status-code'] ?? 'unknown'; + throw new Exception("Repositories list missing in response (status {$status})."); + } + return [ + 'items' => $response['body']['repositories'], + 'total' => $response['body']['total_count'] ?? \count($response['body']['repositories']), + ]; + } + $repositories = []; $currentPage = 1; while (true) { $url = '/installation/repositories'; $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ 'page' => $currentPage, 'per_page' => 100, // Maximum allowed by GitHub API ]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 129-132: The loop break currently checks
count($filteredRepositories) (the post-filtered list) which causes premature
termination when many results are filtered out; change the condition to inspect
the raw page response count instead (the variable holding the unfiltered page
results—e.g. the array populated from the GitHub API response before filtering)
so use count(...) on that raw response variable rather than
$filteredRepositories inside the loop to determine when fewer than 100 items
were returned and to decide to break.
- Around line 107-108: The searchRepositories method currently takes an $owner
parameter that is never used; either implement local owner filtering, document
its deprecation, or remove it—choose one. To implement filtering, update
searchRepositories to, after fetching repositories from the
/installation/repositories response, filter the results by comparing
$repo['owner']['login'] to the $owner value (and handle empty $owner to mean no
filtering), preserving pagination semantics; alternatively add a docstring or
comment to searchRepositories noting that $owner is ignored by the API; or
remove the $owner parameter from the searchRepositories signature and update all
callers to avoid passing owner. Ensure the chosen approach updates the function
signature, callers, and any tests consistently.
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
109-143: Consider optimizing when no search term is provided.When
$searchis empty, the code fetches all pages only to slice the result at the end. For installations with hundreds of repositories, this is inefficient.When no search filtering is needed, you could use the API's native pagination directly:
Suggested optimization
public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array { + $url = '/installation/repositories'; + + // When no search term, use API pagination directly + if (empty($search)) { + $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ + 'page' => $page, + 'per_page' => $per_page, + ]); + + if (!isset($response['body']['repositories'])) { + throw new Exception("Repositories list missing in the response."); + } + + return [ + 'items' => $response['body']['repositories'], + 'total' => $response['body']['total_count'] ?? count($response['body']['repositories']), + ]; + } + + // When searching, fetch all and filter locally $repositories = []; - $currentPage = 1; while (true) { - $url = '/installation/repositories'; $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ // ... rest of the loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/VCS/Adapter/Git/GitHub.php`:
- Around line 109-136: The loop always pages through all repositories even when
$search is empty; change the method in GitHub (the block using $repositories,
$currentPage and the call(self::METHOD_GET, '/installation/repositories', ...))
to short-circuit: if empty($search) make a single API call with the requested
'page' and 'per_page' parameters and return or use that result directly (no
while loop), otherwise keep the existing paging+filtering logic (array_filter on
$response['body']['repositories']) to aggregate pages until fewer than 100 items
are returned; ensure you still throw the same exception when 'repositories' is
missing.
| $repositories = []; | ||
|
|
||
| $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ | ||
| 'q' => "{$search} user:{$owner} fork:true", | ||
| 'page' => $page, | ||
| 'per_page' => $per_page, | ||
| 'sort' => 'updated' | ||
| ]); | ||
| $currentPage = 1; | ||
| while (true) { | ||
| $url = '/installation/repositories'; | ||
| $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [ | ||
| 'page' => $currentPage, | ||
| 'per_page' => 100, // Maximum allowed by GitHub API | ||
| ]); | ||
|
|
||
| if (!isset($response['body']['repositories'])) { | ||
| throw new Exception("Repositories list missing in the response."); | ||
| } | ||
|
|
||
| // Filter repositories to only include those that match the search query. | ||
| $filteredRepositories = array_filter($response['body']['repositories'], fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false); | ||
|
|
||
| // Merge with result so far. | ||
| $repositories = array_merge($repositories, $filteredRepositories); | ||
|
|
||
| if (!isset($response['body']['items'])) { | ||
| throw new Exception("Repositories list missing in the response."); | ||
| // If less than 100 repositories are returned, we have fetched all repositories. | ||
| if (\count($response['body']['repositories']) < 100) { | ||
| break; | ||
| } | ||
|
|
||
| // Increment page number to fetch next page. | ||
| $currentPage++; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance regression: fetches all pages even without a search term.
When $search is empty, this implementation still fetches every page from the API before slicing. For installations with hundreds or thousands of repositories, this creates unnecessary API calls and memory usage.
When no search filtering is needed, delegate pagination directly to the API:
Proposed fix
public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array
{
+ $url = '/installation/repositories';
+
+ // When no search term, delegate pagination directly to the API
+ if (empty($search)) {
+ $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [
+ 'page' => $page,
+ 'per_page' => $per_page,
+ ]);
+
+ if (!isset($response['body']['repositories'])) {
+ throw new Exception("Repositories list missing in the response.");
+ }
+
+ return [
+ 'items' => $response['body']['repositories'],
+ 'total' => $response['body']['total_count'] ?? \count($response['body']['repositories']),
+ ];
+ }
+
+ // When searching, fetch all pages and filter locally
$repositories = [];
-
$currentPage = 1;
while (true) {
- $url = '/installation/repositories';
$response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"], [
'page' => $currentPage,
'per_page' => 100, // Maximum allowed by GitHub API
]);
if (!isset($response['body']['repositories'])) {
throw new Exception("Repositories list missing in the response.");
}
- // Filter repositories to only include those that match the search query.
- $filteredRepositories = array_filter($response['body']['repositories'], fn ($repo) => empty($search) || stripos($repo['name'], $search) !== false);
+ $filteredRepositories = array_filter($response['body']['repositories'], fn ($repo) => stripos($repo['name'], $search) !== false);
- // Merge with result so far.
$repositories = array_merge($repositories, $filteredRepositories);
- // If less than 100 repositories are returned, we have fetched all repositories.
if (\count($response['body']['repositories']) < 100) {
break;
}
- // Increment page number to fetch next page.
$currentPage++;
}🤖 Prompt for AI Agents
In `@src/VCS/Adapter/Git/GitHub.php` around lines 109 - 136, The loop always pages
through all repositories even when $search is empty; change the method in GitHub
(the block using $repositories, $currentPage and the call(self::METHOD_GET,
'/installation/repositories', ...)) to short-circuit: if empty($search) make a
single API call with the requested 'page' and 'per_page' parameters and return
or use that result directly (no while loop), otherwise keep the existing
paging+filtering logic (array_filter on $response['body']['repositories']) to
aggregate pages until fewer than 100 items are returned; ensure you still throw
the same exception when 'repositories' is missing.
Related SER-642
Ensures only the repositories accessible to the installation is returned
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.